-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add icon margin that works on both LTR & RTL, re-add removed styles #104
Add icon margin that works on both LTR & RTL, re-add removed styles #104
Conversation
So all icons will have margin left and margin right, so we dont need browser prefix fixes plus works for IE
*/ | ||
-webkit-margin-start: $inuit-base-spacing-unit--tiny; | ||
-moz-margin-start: $inuit-base-spacing-unit--tiny; | ||
margin-right: $inuit-base-spacing-unit--tiny; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix only can add margin-right value for the dropdown button's icon, the item icon's space issue still exist.
We should add margin-right value for item-icon, but if we only add margin-right, for RTL, the item-icon's space issue will comes out again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshsylvester and @benjaminliugang updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include screenshots of how these changes affect the component?
HISTORY.md
Outdated
@@ -1,3 +1,7 @@ | |||
# v4.9.3 | |||
|
|||
- Use margin right and left to avoid browser specif prefix fix for rtl space issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Use margin right and left to avoid browser specif prefix fix for rtl space issue | |
- Use margin right and left to avoid browser specific prefix fix for rtl space issue |
Also - just repeating again my strong preference for not committing version bumps and HISTORY.md updates in PRs - as both this PR and #105 will create conflicts for each other. Also, judging from the comments, both this and PR #105 treat the same problem? They should probably be part of a single version bump rather than released individually. |
@evanjd Agreed I will combine the 2 PR into 1 so there will be one version change |
@evanjd updated ur comments and combined into one PR. will close the other PR. |
Issue:
The -webkit and -moz fix would repleace margin-left / margin-right depending on the direction of render. But this fix does not work on IE plus unnessary browser prefix in the code.
Styles were removed in this PR
This was causing regression in px-dropdown padding left when it was used in readOnly mode.
Proposed new:
Add margin-right as well , so icon will have margin-right and margin-left, so irrespective of the rendering, text would not be too close to the icon
Re-add back the removed styles